Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shell script to start sencha #434

Merged
merged 15 commits into from
Dec 2, 2024
Merged

shell script to start sencha #434

merged 15 commits into from
Dec 2, 2024

Conversation

riedde
Copy link
Contributor

@riedde riedde commented Oct 9, 2024

Description, Context and related Issue

This PR contains a very small shell script to run a docker containter with sencha for building the edirom

How Has This Been Tested?

I removed the absolute paths and added public valiables. It works on my machine but should work on every machine (Mac).

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • My change requires a change to the documentation.
  • I have performed a self-review of my code

@riedde riedde requested a review from krHERO October 9, 2024 05:16
@riedde
Copy link
Contributor Author

riedde commented Oct 9, 2024

@krHERO To be discussed with the developer team: We can also integrate this script into the build process.

@daniel-jettka
Copy link
Contributor

daniel-jettka commented Oct 10, 2024

@krHERO To be discussed with the developer team: We can also integrate this script into the build process.

I like the idea to integrate this into the build process.
build.sh could test if sencha and ant are available locally and if not then start the build process using the container.

Small addition starts the build process in the container directly:
docker run --rm -it -v `pwd`:/app --name sencha ghcr.io/bwbohl/sencha-cmd:latest /bin/bash -c "./build.sh testing; exit"

What do you think?

@riedde
Copy link
Contributor Author

riedde commented Oct 10, 2024

great idea. I thought something similar. Maybe we can use testing as default if no parameter is set? I have done something similar with the password (for local development use) here: https://github.com/Baumann-Digital/baudi-docker/blob/main/build-run-docker.sh

@peterstadler
Copy link
Member

peterstadler commented Oct 10, 2024

My suggestion would be to not provide this as a shell script but rather add this to the ant build.xml.
Since we already have ant in place I think it'd be beneficial to employ this tool for tying together all the build, test, and run steps.

This also includes current build.sh which we (imho) should get rid off.

@riedde
Copy link
Contributor Author

riedde commented Oct 11, 2024

My intention was to share my way of working, an this was just some addition to build.sh, but I also think we should try to extent the ant build.xml see #440

@riedde riedde marked this pull request as draft October 11, 2024 06:34
@riedde
Copy link
Contributor Author

riedde commented Oct 11, 2024

I converted this to draft until the ant way is explored. I'm intrested in working on that to make a proposal.

@roewenstrunk
Copy link
Member

My suggestion would be to not provide this as a shell script but rather add this to the ant build.xml. Since we already have ant in place I think it'd be beneficial to employ this tool for tying together all the build, test, and run steps.

This also includes current build.sh which we (imho) should get rid off.

It is an interesting question which tools we expect developers to use. Benni's Docker image has Ant integrated. It would therefore be possible to develop on the Edirom without having Ant installed locally.

@riedde
Copy link
Contributor Author

riedde commented Oct 11, 2024 via email

@roewenstrunk
Copy link
Member

Are we talking about the same? I am talking about Benni‘s Sencha-Image not about his Edirom-Image. So what about a short discussion on this at the next developer’s meeting? Maybe we already have some good Workflows and just have to share or combine them?

I was talking about the Sencha-CMD image, too. There is Ant integrated. So technically you don't need Ant locally installed to build Edirom Online.

@riedde
Copy link
Contributor Author

riedde commented Oct 11, 2024

Are we talking about the same? I am talking about Benni‘s Sencha-Image not about his Edirom-Image. So what about a short discussion on this at the next developer’s meeting? Maybe we already have some good Workflows and just have to share or combine them?

I was talking about the Sencha-CMD image, too. There is Ant integrated. So technically you don't need Ant locally installed to build Edirom Online.

Now, I got it. I guess this is why my first proposal was to integrate the docker commands into build.sh

@roewenstrunk
Copy link
Member

@krHERO To be discussed with the developer team: We can also integrate this script into the build process.

I like the idea to integrate this into the build process. build.sh could test if sencha and ant are available locally and if not then start the build process using the container.

Small addition starts the build process in the container directly: docker run --rm -it -v pwd:/app --name sencha ghcr.io/bwbohl/sencha-cmd:latest /bin/bash -c "./build.sh testing; exit"

What do you think?

@riedde, if you follow Daniel's suggestion, the docker command can't be integrated into build.sh because build.sh is called inside the docker container. So you would need another shell script for calling docker.

@riedde
Copy link
Contributor Author

riedde commented Oct 11, 2024

Thanks for the hint! I'm going to investigate some solutions and present them at the next meeting.

@daniel-jettka
Copy link
Contributor

Proposing small change to current script

  • flag "-d" starts docker container and in there the script is executed in the former way

@daniel-jettka daniel-jettka marked this pull request as ready for review November 7, 2024 08:15
@riedde
Copy link
Contributor Author

riedde commented Nov 7, 2024

Tested on my machine. Works fine! Thank you @daniel-jettka for your help!

@peterstadler peterstadler force-pushed the ftr/shell-to-start-sencha branch from 014dd78 to 573c73b Compare November 8, 2024 09:43
@peterstadler
Copy link
Member

@riedde please do not hit the "update branch" button and merge dev into this branch. This will create "ugly" history for it inserts all those unnecessary merge commits. It's better to rebase the branch but beware that you'll need to eventually reset hard your local clone.

@riedde
Copy link
Contributor Author

riedde commented Nov 8, 2024

@peterstadler Okay, I won't use this button again, but rebasing is not my favourit way of working. It cost a lot of time because there are always conflicts that are mostly hard to handle. Especially when there is to much code formatting.

build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
build.sh Show resolved Hide resolved
@peterstadler peterstadler added this to the 1.0.0 milestone Nov 8, 2024
@peterstadler peterstadler self-assigned this Nov 8, 2024
@peterstadler
Copy link
Member

@daniel-jettka could you have a look at the code suggestions and whether this works for you?

@daniel-jettka
Copy link
Contributor

@daniel-jettka could you have a look at the code suggestions and whether this works for you?

see above :-)

@daniel-jettka
Copy link
Contributor

daniel-jettka commented Nov 28, 2024

@peterstadler @riedde What do you think, can this be merged now?

Copy link
Member

@peterstadler peterstadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is great now but yet another PR with several unnecessary and history polluting merges …

@riedde
Copy link
Contributor Author

riedde commented Dec 2, 2024

The code is great now but yet another PR with several unnecessary and history polluting merges …

@peterstadler Could you bring this up at the next community meeting? I think everyone is doing update, rebase and merge in his/her own style at the moment.

@hizclick hizclick merged commit d35d75a into develop Dec 2, 2024
4 of 5 checks passed
@hizclick hizclick deleted the ftr/shell-to-start-sencha branch December 2, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants